feat(mcp): chart type plugin registry for extensible generate_chart#39922
feat(mcp): chart type plugin registry for extensible generate_chart#39922aminghadersohi wants to merge 6 commits intomasterfrom
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The inline context is insufficient to determine what 'this' refers to in the question. No diff hunk or suggestion snippet is provided for the thread in superset/mcp_service/chart/chart_utils.py. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a chart-type plugin architecture for the MCP chart pipeline, centralizing per-chart-type behavior (pre-validation, column extraction/normalization, form_data mapping, runtime warnings, naming, viz_type resolution) into one plugin per supported chart_type. This reduces the previous need to update multiple dispatch sites when adding or modifying chart types.
Changes:
- Added a
ChartTypePluginprotocol +BaseChartPlugin, a global plugin registry, and 7 built-in chart plugins (xy,table,pie,pivot_table,mixed_timeseries,handlebars,big_number). - Refactored schema/dataset/runtime validation and form_data mapping to dispatch via the registry instead of
isinstance/if/elifchains. - Enhanced tool/docs output by expanding
generate_chartdocstring chart types and addingchart_type_display_nametoChartInfo.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| superset/mcp_service/chart/validation/schema_validator.py | Switch pre-validation dispatch to registry/plugins; refactor Pydantic error enhancement. |
| superset/mcp_service/chart/validation/runtime/init.py | Replace XY-only runtime checks with per-plugin runtime warning dispatch. |
| superset/mcp_service/chart/validation/dataset_validator.py | Delegate column extraction/normalization to plugins; apply aggregation validation for all chart types. |
| superset/mcp_service/chart/tool/generate_chart.py | Update tool docstring to list all supported chart_type values. |
| superset/mcp_service/chart/schemas.py | Add chart_type_display_name to ChartInfo; resolve display name via registry mapping. |
| superset/mcp_service/chart/registry.py | Introduce global registry for chart plugins + viz_type display-name lookup. |
| superset/mcp_service/chart/plugin.py | Add plugin protocol and base class defining the plugin responsibilities. |
| superset/mcp_service/chart/plugins/init.py | Register the built-in plugins on import. |
| superset/mcp_service/chart/plugins/xy.py | Implement XY plugin (pre-validate, refs, mapping, naming, viz_type, runtime warnings). |
| superset/mcp_service/chart/plugins/table.py | Implement Table plugin. |
| superset/mcp_service/chart/plugins/pie.py | Implement Pie plugin. |
| superset/mcp_service/chart/plugins/pivot_table.py | Implement Pivot Table plugin. |
| superset/mcp_service/chart/plugins/mixed_timeseries.py | Implement Mixed Timeseries plugin. |
| superset/mcp_service/chart/plugins/handlebars.py | Implement Handlebars plugin. |
| superset/mcp_service/chart/plugins/big_number.py | Implement Big Number plugin including post-map trendline temporal validation. |
| superset/mcp_service/chart/chart_utils.py | Replace mapping/name/viz_type dispatch with registry/plugin calls. |
| superset/mcp_service/app.py | Import plugins at MCP app startup; update default instructions about chart type naming. |
…generation Replaces four scattered dispatch locations (schema_validator, dataset_validator, chart_utils, runtime validator) with a central ChartTypePlugin registry. Each of the 7 supported chart types (xy, table, pie, pivot_table, mixed_timeseries, handlebars, big_number) now owns its pre-validation, column extraction, form_data mapping, post-map validation, column normalization, and runtime warnings in a single plugin class. Key changes: - Add ChartTypePlugin protocol and BaseChartPlugin base class (plugin.py) - Add ChartTypeRegistry with register/get/all_types helpers (registry.py) - Add 7 chart type plugins under chart/plugins/ with full coverage - Fix 5-type column validation gap: pie, pivot_table, mixed_timeseries, handlebars, and big_number now participate in dataset column validation (previously silently skipped) - Move BigNumber trendline temporal check to BigNumberChartPlugin.post_map_validate() - Add get_runtime_warnings() to plugin protocol; XYChartPlugin implements format/cardinality checks, removing isinstance(config, XYChartConfig) from RuntimeValidator - Fix stale generate_chart.py docstring listing only 'xy' and 'table' chart types - Add missing pie, pivot_table, mixed_timeseries handlers to _enhance_validation_error; refactor into a data-driven lookup table to stay within complexity limits - Fix empty details fallback in Pydantic error handler
Each ChartTypePlugin now declares:
- display_name: human-readable label for the chart_type discriminator
(e.g. "Line / Bar / Area / Scatter Chart", "Pivot Table")
- native_viz_types: dict mapping every Superset-internal viz_type the
plugin produces to a user-friendly name
(e.g. {"echarts_timeseries_line": "Line Chart", "echarts_area": "Area Chart"})
The registry gains display_name_for_viz_type(viz_type) which searches
all plugins' native_viz_types maps, replacing the need for a separate
viz_type_display_names.json or viz_type_names.py module.
ChartInfo gains a chart_type_display_name field populated via the registry,
so list_charts / get_chart_info return human-readable chart type names.
The MCP system instructions now reference display names rather than
internal viz_type identifiers.
H1: Delete 7 dead _pre_validate_* static methods from SchemaValidator
— exact duplicates of plugin pre_validate() methods, never called
after _pre_validate_chart_type() was updated to delegate to plugin.
H2: Inline DatasetValidator._normalize_xy_config/_normalize_table_config
into XYChartPlugin/TableChartPlugin.normalize_column_refs() and delete
both DatasetValidator helper methods. The 5 other plugins already
called _get_canonical_column_name directly; XY and Table now match.
H3: Add generate_name()/resolve_viz_type() to ChartTypePlugin protocol
and BaseChartPlugin, implement in all 7 plugins. Replace the 7-arm
isinstance chain in generate_chart_name() and the 7-arm elif chain
in _resolve_viz_type() with single-line registry dispatch.
H4: Add a sync comment above _CHART_TYPE_ERROR_HINTS to document that
it must stay in sync with the plugin registry.
M4: Move logger=logging.getLogger(__name__) from inside
XYChartPlugin.get_runtime_warnings() to module scope.
…xes, test repairs On top of the dead-code elimination in the previous commit: - Add lazy _ensure_plugins_loaded() bootstrap to ChartTypeRegistry so the registry is populated even without importing app.py (fixes isolated test runs) - Delegate _RegistryProxy methods to module-level functions so bootstrap runs - Guard register() against empty chart_type strings - Add generate_name + resolve_viz_type to ChartTypePlugin Protocol and BaseChartPlugin; delegate generate_chart_name/_resolve_viz_type in chart_utils to the plugin registry - Add _with_context static helper to BaseChartPlugin (shared by all plugins) - Fix stale 'five methods' → 'eight methods' docstring in plugin.py - Add TypeVar _C to normalize_column_names so mypy infers correct return type - Fix broken tests: update _pre_validate_big_number_config → _pre_validate_chart_type, remove deleted TestNormalizeXYConfig/TestNormalizeTableConfig classes, update runtime validator tests for removed _validate_format_compatibility / _validate_cardinality methods, add x is not None narrowing guards Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nal corrections, cardinality suggestions - Remove redundant local imports from BigNumberChartPlugin.post_map_validate() now that BigNumberChartConfig and is_column_truly_temporal are at top level - Add explanatory comments on the two remaining local get_registry imports in chart_utils.py and dataset_validator.py (circular import prevention) - Fix schema_validator.py and generate_chart.py docstring: XY 'x' field is optional (defaults to dataset primary datetime column), not required - Propagate cardinality suggestions alongside warnings in XYChartPlugin - Clarify app.py instructions: chart_type_display_name is null for viz_types outside the 7 generate_chart-supported types Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All per-method local imports in the 7 chart plugins were moved to module-level. None of them create circular imports: schemas.py, chart_utils.py, and dataset_validator.py are safe to import at plugin load time because those modules guard their own registry lookups with local imports. - big_number: add map_big_number_config, _big_number_chart_what, _summarize_filters, DatasetValidator to top-level imports - pie: add map_pie_config, _pie_chart_what, _summarize_filters, PieChartConfig, DatasetValidator to top-level imports - xy: add map_xy_config, _xy_chart_what/context, XYChartConfig, DatasetValidator, FormatTypeValidator, CardinalityValidator to top-level imports - table: add map_table_config, _table_chart_what, _summarize_filters, TableChartConfig, DatasetValidator to top-level imports - pivot_table: add map_pivot_table_config, _pivot_table_what, _summarize_filters, PivotTableChartConfig, DatasetValidator to top-level imports - mixed_timeseries: add map_mixed_timeseries_config, _mixed_timeseries_what, _summarize_filters, MixedTimeseriesChartConfig, DatasetValidator to top-level - handlebars: add map_handlebars_config, _handlebars_chart_what, _summarize_filters, HandlebarsChartConfig, DatasetValidator to top-level imports Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
891676f to
1e2b541
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39922 +/- ##
==========================================
+ Coverage 63.88% 63.90% +0.02%
==========================================
Files 2583 2593 +10
Lines 136618 136938 +320
Branches 31502 31513 +11
==========================================
+ Hits 87272 87504 +232
- Misses 47830 47916 +86
- Partials 1516 1518 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SUMMARY
Introduces a
ChartTypePluginprotocol and central registry that consolidates all chart-type-specific logic into a single plugin class per chart type. Previously, adding a new chart type required synchronised edits to four separate dispatch locations. With this change it requires one new file.The problem: 4 dispatch points that had to stay in sync
Before this PR,
generate_chartlogic was split across:schema_validator.py_pre_validate_*static methods, one per typedataset_validator.pyisinstancebranches in_extract_column_referencesandnormalize_column_nameschart_utils.pyif/elifchain inmap_config_to_form_data,generate_chart_name,_resolve_viz_typeruntime/__init__.pyisinstance(config, XYChartConfig)hardcoded branchResult: 5 of 7 chart types (
pie,pivot_table,mixed_timeseries,handlebars,big_number) silently skipped column validation — a bug discovered only when charts failed at query time.Architecture
New file layout
Request flow (before → after)
flowchart LR subgraph BEFORE["Before: 4 separate dispatch points"] direction TB A1[schema_validator.py\n7 pre_validate methods] A2[dataset_validator.py\nisinstance branches] A3[chart_utils.py\nif/elif chains] A4[runtime/__init__.py\nXY hardcoding] end subgraph AFTER["After: single registry lookup"] direction TB B1[registry.get chart_type] --> B2[plugin.pre_validate] B1 --> B3[plugin.extract_column_refs] B1 --> B4[plugin.to_form_data] B1 --> B5[plugin.post_map_validate] B1 --> B6[plugin.normalize_column_refs] B1 --> B7[plugin.get_runtime_warnings] B1 --> B8[plugin.generate_name] B1 --> B9[plugin.resolve_viz_type] end BEFORE -.->|refactored to| AFTERPlugin protocol
classDiagram class ChartTypePlugin { <<Protocol>> +str chart_type +str display_name +dict native_viz_types +pre_validate(config) ChartGenerationError|None +extract_column_refs(config) list[ColumnRef] +to_form_data(config, dataset_id) dict +post_map_validate(config, form_data, dataset_id) ChartGenerationError|None +normalize_column_refs(config, dataset_context) Any +get_runtime_warnings(config, dataset_id) list[str] +generate_name(config, dataset_name) str +resolve_viz_type(config) str } class BaseChartPlugin { +pre_validate() None +extract_column_refs() [] +to_form_data() NotImplementedError +post_map_validate() None +normalize_column_refs() config unchanged +get_runtime_warnings() [] +generate_name() "Chart" +resolve_viz_type() "unknown" +_with_context(what, context)$ str } class XYChartPlugin class PieChartPlugin class TableChartPlugin class BigNumberChartPlugin class PivotTableChartPlugin class MixedTimeseriesChartPlugin class HandlebarsChartPlugin ChartTypePlugin <|.. BaseChartPlugin BaseChartPlugin <|-- XYChartPlugin BaseChartPlugin <|-- PieChartPlugin BaseChartPlugin <|-- TableChartPlugin BaseChartPlugin <|-- BigNumberChartPlugin BaseChartPlugin <|-- PivotTableChartPlugin BaseChartPlugin <|-- MixedTimeseriesChartPlugin BaseChartPlugin <|-- HandlebarsChartPluginRegistry bootstrap (lazy, no circular imports)
sequenceDiagram participant Caller as Caller (test / tool) participant Registry as registry.py participant Plugins as plugins/__init__.py Caller->>Registry: get("xy") Registry->>Registry: _ensure_plugins_loaded() alt first call Registry->>Plugins: import (triggers register() calls) Plugins-->>Registry: 7 plugins registered end Registry-->>Caller: XYChartPlugin instanceThe registry self-bootstraps on first lookup — no dependency on
app.pybeing imported first, so isolated unit tests work without the full Flask app.How to add a new chart type
Everything goes in one new file. Here is a minimal working example for a hypothetical
funnelchart:Step 1 — Add the Pydantic schema (
chart/schemas.py):Step 2 — Create the plugin (
chart/plugins/funnel.py):Step 3 — Register it (
chart/plugins/__init__.py):Step 4 — Add a Pydantic error hint (
chart/validation/schema_validator.py):That is the complete change set — no edits to
chart_utils.py,dataset_validator.py, orruntime/__init__.pydispatch logic.Bug fixes included
pie,pivot_table,mixed_timeseries,handlebars, andbig_numbercharts previously skipped dataset column validation entirely (silent false-pass). All 7 types now run column existence and aggregation checks.BigNumberChartPlugin.post_map_validate().isinstance(config, XYChartConfig)hardcoding fromRuntimeValidator; XY-specific format/cardinality checks now live inXYChartPlugin.get_runtime_warnings().generate_chart.pylisted only'xy'and'table'as valid chart types; updated to list all 7.pie,pivot_table,mixed_timeseriescases to_enhance_validation_error; refactored to a data-driven lookup table to reduce cyclomatic complexity.app.pyget a fully populated registry.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change, no UI modifications.
TESTING INSTRUCTIONS
Manual verification:
python -m superset.mcp_service.servercolumn_not_founderror with fuzzy suggestions (previously: silent pass, runtime failure)big_numberchart withshow_trendline=truebut notemporal_column→ should error at validationgenerate_chartfor all 7 chart types to confirm no regressionsADDITIONAL INFORMATION